Add setup-k8s.sh: generic Kubernetes install path#142
Conversation
pdettori
left a comment
pdettori
left a comment
There was a problem hiding this comment.
Summary
Purely additive PR (two new files: setup-k8s.sh + README-k8s.md) adding a generic-Kubernetes install path that sits between setup-kind.sh and setup-ocp.sh. Nicely structured: reuses the shared base manifests via runtime sed substitution (no forked YAMLs), set -euo pipefail, logs to stderr so --dry-run emits clean pipeable YAML, and mirrors the sibling scripts' shape. All CI green (shellcheck, hadolint, DCO, Trivy, CodeQL).
One functional break (blocking) and one portability suggestion below.
Author: moonlight16 (MEMBER — maintainer)
Areas reviewed: Shell, YAML/manifests, Docs
Commits: 2, all signed-off
CI: all passing
must-fix — kubectl set env on a Knative Service
Step 7 sets the model env with kubectl set env ksvc/serverless-harness. set env only supports built-in workload types; on the Knative Service CRD it fails with no kind "Service" registered for serving.knative.dev/v1. Under set -euo pipefail this aborts the script at its final step whenever SH_MODEL or SH_MODEL_CUSTOM is set — which is exactly the "Choosing the model" / self-hosted-endpoint path the README documents. Both setup-kind.sh and setup-ocp.sh avoid this by using kubectl patch ksvc --type merge.
suggestion — non-portable \n in the --storage-class sed
The storage-class injection relies on \n in the sed replacement, which only expands to a newline under GNU sed. On BSD/macOS sed it becomes a literal n and corrupts the YAML. Since --storage-class ibm-scale-csi is a headline feature and this script commonly runs from a workstation, a portable insertion would be safer. Default-storage path is unaffected.
Assisted-By: Claude Code
| ENV_SETS=() | ||
| [ -n "${SH_MODEL:-}" ] && ENV_SETS+=("SH_MODEL=${SH_MODEL}") | ||
| [ -n "${SH_MODEL_CUSTOM:-}" ] && ENV_SETS+=("SH_MODEL_CUSTOM=${SH_MODEL_CUSTOM}") | ||
| [ "${#ENV_SETS[@]}" -gt 0 ] && "${KUBECTL[@]}" -n "$NAMESPACE" set env ksvc/serverless-harness "${ENV_SETS[@]}" |
There was a problem hiding this comment.
must-fix: kubectl set env ksvc/... fails on the Knative Service CRD — set env only supports built-in workload types, and on a ksvc it errors with no kind "Service" registered for serving.knative.dev/v1. Because of set -euo pipefail, this aborts the whole script at the final step whenever SH_MODEL/SH_MODEL_CUSTOM is set — precisely the "Choosing the model" path the README documents.
Both siblings use kubectl patch ksvc --type merge instead. Two clean fixes:
SH_MODELis already a literal env inservice.yaml, so substitute it inrender_base()alongside the image/namespace subs.- For
SH_MODEL_CUSTOM(not inservice.yaml), use a JSON-patchreplaceon/spec/template/spec/containers/0/envthat upserts by name and preserves theANTHROPIC_*secretKeyRefs — a plain merge-patch would drop them.
There was a problem hiding this comment.
Looks addressed by commit 703e04f (pushed after this review): the script no longer uses kubectl set env on the ksvc. SH_MODEL/SH_MODEL_CUSTOM are now injected declaratively in render_base(), and the SH_MODEL_CUSTOM path uses an awk insert (not a merge-patch), so the ANTHROPIC_* secretKeyRefs are preserved — exactly your point. Verified against the real service.yaml. Leaving this thread for you to confirm/resolve.
| | if [ -n "$STORAGE_CLASS" ]; then | ||
| # Insert storageClassName above accessModes in each volumeClaimTemplate PVC spec | ||
| # (only the sandbox pool has these). Preserves the existing 8-space indent. | ||
| sed -e "s#^\( *\)accessModes: \[\"ReadWriteOnce\"\]#\1storageClassName: ${STORAGE_CLASS}\n\1accessModes: [\"ReadWriteOnce\"]#g" |
There was a problem hiding this comment.
suggestion: the \n in this sed replacement only expands to a newline under GNU sed. On BSD/macOS sed it inserts a literal n, producing malformed YAML (storageClassName: ibm-scale-csinaccessModes: ["ReadWriteOnce"]). Since this script typically runs from a workstation targeting a remote cluster and --storage-class ibm-scale-csi (GPFS) is a headline feature, consider a portable insertion (e.g. awk, or a printf-built newline). The default-storage path is unaffected.
There was a problem hiding this comment.
Addressed in 703e04f — the storage-class injection now uses awk instead of sed \n, so it's portable across GNU/BSD sed. Leaving this for you to confirm/resolve.
The repo has setup-kind.sh (local Kind) and setup-ocp.sh (OpenShift) but no path for a vanilla Kubernetes cluster (k3s, EKS, GKE, on-prem, ...): setup-kind.sh assumes single-node Kind and builds+kind-loads the image; setup-ocp.sh assumes OpenShift OLM operators and auto-created Routes. Neither fits a generic cluster. Add setup-k8s.sh installing the same stack (Knative Serving + Kourier, agent-sandbox controller, Redis, sandbox pool, LLM secret, harness ksvc) on any Kubernetes cluster, reusing the shared base manifests with cluster-specifics injected via flags (mirrors setup-ocp.sh's render_overlay_dir sed approach — no forked YAMLs). Plus README-k8s.md. Knobs: --namespace, --image/--sandbox-image (prebuilt refs; any registry incl. in-cluster), --storage-class (default: cluster default; e.g. ibm-scale-csi for GPFS), --ingress none|nodeport, --with-keda, --context, --dry-run. Model via SH_MODEL/SH_MODEL_CUSTOM. Logs go to stderr so --dry-run stdout is clean YAML. Purely additive: two new files, no changes to existing scripts or manifests. Derived from a working k3s deployment (ran BugStone Acts 1 & 2 end-to-end). Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Jeremy Cohn <Jeremy.Cohn@ibm.com>
Double-quote ${KUBECTL[@]} in the startup log line to avoid word re-splitting,
per the repo's shellcheck CI (SC2068).
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Jeremy Cohn <Jeremy.Cohn@ibm.com>
Address review feedback from pdettori on rossoctl#142: - kubectl set env does not work on a Knative Service (not a built-in workload kind recognized by that subcommand), so setting SH_MODEL or SH_MODEL_CUSTOM aborted the whole script under set -euo pipefail. Fix: inject both declaratively into service.yaml via render_base(), matching how every other cluster-specific value is already handled. - The --storage-class insertion used sed's \n in a replacement string, which only expands to a newline under GNU sed; on BSD/macOS sed it inserts a literal 'n', producing malformed YAML. Fix: use awk with match()/substr() for indent-preserving line insertion instead. Validated via --dry-run across the custom-model+GPFS path, the SH_MODEL-only path, the default (no-flags) path, and namespace+storage- class combined — all render valid YAML with correct substitutions and no cross-contamination between flags. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Jeremy Cohn <Jeremy.Cohn@ibm.com>
3b2014e to
703e04f
Compare
mrsabath
left a comment
mrsabath
left a comment
There was a problem hiding this comment.
New generic-Kubernetes install path (two new files). @pdettori requested changes at commit 3b2014e; the author then pushed 703e04f which addresses both of his points — his threads are now outdated. I verified the fixes against the real base manifest.
pdettori's findings — both resolved on current HEAD:
- must-fix (
kubectl set env ksvc/...errors on the Knative Service CRD): the script no longer usesset env.SH_MODEL/SH_MODEL_CUSTOMare now injected declaratively inrender_base(), with an explicit comment (L435–437) on why. Verified againstservice.yaml(- name: SH_MODEL/value: "claude-haiku-4-5"): the sed value-swap and the awk insertion both match the real structure, and the awk inserts a new env entry rather than patching, so theANTHROPIC_*secretKeyRefs are preserved — exactly the concern raised. - suggestion (
sed \nis GNU-only): storage-class injection now usesawkwith portable indent computation.
Additional checks: set -euo pipefail present; no secret values logged (only names, in an error message); Knative/KEDA versions overridable and agent-sandbox pinned to v0.5.0; shellcheck CI green; DCO signed.
One trivial nit inline. Verdict: APPROVE — the blocking issues are fixed and verified. (Note: @pdettori's existing CHANGES_REQUESTED still stands until he re-reviews or dismisses it.)
Areas reviewed: Shell, YAML rendering, docs, security. Commits: 3, all signed-off. CI: passing.
| else cat; fi \ | ||
| | if [ -n "${SH_MODEL:-}" ]; then | ||
| # service.yaml declares `- name: SH_MODEL` / `value: "claude-haiku-4-5"`; swap the value. | ||
| sed -e "s#\(- name: SH_MODEL\$\)#\1#" \ |
There was a problem hiding this comment.
nit: this sed -e "s#\(- name: SH_MODEL\$\)#\1#" replaces the match with itself — a no-op. The next line's /- name: SH_MODEL$/{n;s#value: "..."#...#;} does the actual value swap, so this line can be dropped. Harmless as-is.
The `s#\(- name: SH_MODEL$\)#\1#` expression replaced the match with itself; the following `-e` does the actual value swap. Addresses mrsabath's L146 nit on rossoctl#142. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Jeremy Cohn <Jeremy.Cohn@ibm.com>
3 participants
Add a generic Kubernetes install script, filling the gap between
setup-kind.sh(local Kind) andsetup-ocp.sh(OpenShift).Why
Deploying on a real, non-OpenShift Kubernetes cluster (k3s, EKS, GKE, on-prem, …) fits neither existing script:
setup-kind.shassumes a local single-node Kind cluster and builds +kind loads the image — no good on a remote cluster.setup-ocp.shassumes OpenShift-specific machinery: OLM operators (OpenShift Serverless, Custom Metrics Autoscaler) and auto-created Routes — none of which exist on vanilla k8s.Today that means hand-porting one of them. This adds the missing generic path.
What
setup-k8s.shinstalls the same stack (Knative Serving + Kourier, agent-sandbox controller, Redis, sandbox pool, LLM secret, harness ksvc) on any Kubernetes cluster, reusing the shared base manifests with cluster-specifics injected via flags — mirroringsetup-ocp.sh’srender_overlay_dirsed approach, so nothing is forked. PlusREADME-k8s.md.How the three scripts differ:
setup-kind.shsetup-k8s.sh(new)setup-ocp.shkind load--imagerefs (in-cluster build documented)oc new-builddefault--namespace--namespace--storage-class(default: cluster default)--ingress nodeportKnobs:
--namespace,--image/--sandbox-image,--storage-class(incl.ibm-scale-csifor GPFS),--ingress,--with-keda,--context,--dry-run. Model viaSH_MODEL/SH_MODEL_CUSTOM.Why it is safe
--dry-runemits clean, pipeable YAML.Notes
SH_MODEL_CUSTOM) if targeting a self-hosted model, and Spread sandbox pool pods across nodes (topologySpreadConstraints) #134 (pool node-spread); both are independent and this works without them (the extra env is simply ignored).setup-ocp.shcan assume, but vanilla k8s has none, so this stays builder-agnostic and consumes a prebuilt ref. README documents local and in-cluster (Shipwright/Tekton) rebuild patterns.Testing
Derived from a working k3s deployment (this stack ran BugStone Acts 1 & 2 end-to-end).
--dry-runrenders valid manifests for both default-storage and--storage-class ibm-scale-csi;bash -nclean.